-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Small cleanup in mercury v0.3 request code #11442
Conversation
I see that you haven't updated any CHANGELOG files. Would it make sense to do so? |
c.threadCtrl.Go(func(ctx context.Context) { | ||
c.multiFeedsRequest(ctx, ch, streamsLookup) | ||
}) | ||
|
||
var reqErr error | ||
var retryInterval time.Duration | ||
results := make([][]byte, len(streamsLookup.Feeds)) | ||
retryable := true | ||
allSuccess := true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no longer need to have logic for allSuccess since we don't have a loop and its based on single result
c.threadCtrl.Go(func(ctx context.Context) { | ||
c.multiFeedsRequest(ctx, ch, streamsLookup) | ||
}) | ||
|
||
var reqErr error | ||
var retryInterval time.Duration | ||
results := make([][]byte, len(streamsLookup.Feeds)) | ||
retryable := true | ||
allSuccess := true | ||
retryable := false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no longer need to start with true and keep doing && within loop
If the single request fails then we'll set the retryable value, else it's false by default
if len(streamsLookup.Feeds) == 0 { | ||
return mercury.NoPipelineError, mercury.MercuryUpkeepFailureReasonInvalidRevertDataInput, [][]byte{}, false, 0 * time.Second, fmt.Errorf("invalid revert data input: feed param key %s, time param key %s, feeds %s", streamsLookup.FeedParamKey, streamsLookup.TimeParamKey, streamsLookup.Feeds) | ||
} | ||
resultLen = 1 | ||
resultLen := 1 // Only 1 multi-feed request is made for all feeds | ||
ch := make(chan mercury.MercuryData, resultLen) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previously a larger size channel was being made than was necessary (only 1 value was written to it). Reducing this shouldn't have any effect
AUTO-7930
Doing a minor cleanup in mercury v0.3 code post refactor
Since it only does one request, it doesn't need to wait for results in a loop. Aggregation of retryable/success values also becomes easier as no aggregation is required, rather it just depends on a single value now
Test plan:
Rely on exisitng CI tests for mercury 0.3 path and also code review as it's a small change